Skip to content

Comments

eclib: 20190909 -> 20210625, import sage update patch#123692

Merged
SuperSandro2000 merged 1 commit intoNixOS:masterfrom
collares:eclib-20210503
Jul 5, 2021
Merged

eclib: 20190909 -> 20210625, import sage update patch#123692
SuperSandro2000 merged 1 commit intoNixOS:masterfrom
collares:eclib-20210503

Conversation

@collares
Copy link
Member

@collares collares commented May 19, 2021

Motivation for this change

Package update.

This employs the same "fetchpatch to fetch a diff from sagetrac-mirror" strategy used in #122624 because the eclib Sage update ticket was positively reviewed but has not been merged yet. I am not in a hurry to get this in 21.05 because I think few (if any) people use eclib outside Sage, and since Sage 9.3 ships with eclib 20190909 no one expects a more recent version. But I am not the best person to decide this, so here's a PR to make all options available.

Fixes #114960.

(I didn't check if the diff/patch distinction matters this time. .diff just seemed better because there were 20-ish commits in the Trac ticket.)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

This was referenced May 19, 2021
@ofborg ofborg bot requested review from 7c6f434c, omasanori and timokau May 19, 2021 15:02
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels May 19, 2021
@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 123692 at b101dcff run on x86_64-linux 1

2 packages skipped due to time constraints:
  • sage
  • sageWithDoc
2 packages built successfully:
  • eclib
  • tests.trivial

Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The update looks non-trivial. It has been a while since the last one, which might make bugs or other potentially breaking changes in behavior more likely. I would tend towards waiting a bit longer.

You're more in touch with the upstream (sage) developments than I am these days, so I actually do think you're (one of the) best person(s) to decide this. Since you don't seem to be opposed to waiting, let's wait a bit.

Edit: "A bit" is pretty vague. I'll let you decide what exactly that means here ;) Feel free to ping me when you think it is time to reconsider this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! I think I'll ping again you once this reaches a Sage beta.

The main reason for the change is that v20210503 is not tagged. Combined with the fact that eclib release tarballs are now manually uploaded by the developer (no longer just a repository snapshot automatically created by GitHub), it looked like the most authoritative source.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll ping again you once this reaches a Sage beta.

👍

The main reason for the change is that v20210503 is not tagged. Combined with the fact that eclib release tarballs are now manually uploaded by the developer (no longer just a repository snapshot automatically created by GitHub), it looked like the most authoritative source.

I see, thank you for clarifying. It would be nice to add this information to the package declaration. I like to put information like "releases not tagged, update hash on src on update" near the version attribute where it is easy to spot when someone tries to update (and visible in the diff when reviewing and update).

A comment about the manually uploaded release tarballs would also be nice to have.

Copy link
Member

@SuperSandro2000 SuperSandro2000 May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we ping upstream that they start to tag the releases again? This just makes downstream life harder.

@collares collares changed the title eclib: 20190909 -> 20210503, import sage update patch eclib: 20190909 -> 20210625, import sage update patch Jul 2, 2021
@ofborg ofborg bot requested a review from timokau July 2, 2021 20:16
@collares
Copy link
Member Author

collares commented Jul 3, 2021

Result of nixpkgs-review pr 123692 run on x86_64-linux 1

3 packages built:
  • eclib
  • sage (sagemath)
  • sageWithDoc

@SuperSandro2000 SuperSandro2000 merged commit 90fa755 into NixOS:master Jul 5, 2021
@collares collares deleted the eclib-20210503 branch December 20, 2021 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants